systemtests: add per-test max_time override for precice-config #744
systemtests: add per-test max_time override for precice-config #744AdityaGupta716 wants to merge 10 commits intoprecice:developfrom
Conversation
|
@MakisH Plz review |
|
This pull request has been mentioned on preCICE Forum on Discourse. There might be relevant details there: https://precice.discourse.group/t/gsoc-2026-aditya-gupta/2773/4 |
MakisH
added
GSoC
|
How does this PR relate to the previously opened #738? |
|
@MakisH yes PR tackles the same issue as #738. I opened it separately because I wanted to keep the scope focused just the max_time override, without the iterations-log changes that #738 also includes. |
MakisH
left a comment
MakisH
left a comment
There was a problem hiding this comment.
FYI, this PR (and some other) could benefit from:
- A short explanation of the main idea:
- Reads a
max-timevalue fromtests.yaml - Overwrites the value in the
precice-config.xmlwhile preparing the case to run.
- Reads a
- Some Markdown formatting, especially where you show code.
Given my previous comment on the relation to another PR (#744 (comment)), note that I still need to review the other PR as well. Maybe don't rush implementing everything.
| @@ -0,0 +1 @@ | |||
| - Add per-test `max_time` override in `tests.yaml` to cap preCICE simulation time without editing `precice-config.xml` manually. Applies consistently to both test runs and reference result generation. Handles multiple `<max-time>` tags with a warning. | |||
There was a problem hiding this comment.
Handles multiple
<max-time>tags with a warning.
This should be a warning in preCICE library itself, not in the system tests.
|
|
||
|
|
||
| GLOBAL_TIMEOUT = 900 | ||
| BUILD_TIMEOUT = 900 |
| arguments: SystemtestArguments | ||
| case_combination: CaseCombination | ||
| reference_result: ReferenceResult | ||
| max_time: Optional[float] = None |
There was a problem hiding this comment.
Optionl seems to be an old practice: https://stackoverflow.com/a/51710151/2254346
But I am not very up-to-date in Python.
|
|
||
| try: | ||
| stdout, stderr = process.communicate(timeout=GLOBAL_TIMEOUT) | ||
| stdout, stderr = process.communicate(timeout=self.timeout) |
| config_path = self.system_test_dir / "precice-config.xml" | ||
| if not config_path.exists(): | ||
| logging.warning( | ||
| f"Requested max_time override for {self}, but no precice-config.xml " | ||
| f"found in {self.system_test_dir}") | ||
| return |
There was a problem hiding this comment.
This should never happen in the system tests: all tests have a precice-config.xml, by definition. I would remove it to keep the code simple.
| try: | ||
| text = config_path.read_text() | ||
| except Exception as e: | ||
| logging.warning(f"Could not read {config_path} to apply max_time override: {e}") | ||
| return | ||
| pattern = r'(<max-time\s+value=")([^"]*)(")' | ||
| matches = re.findall(pattern, text) | ||
| if not matches: | ||
| logging.warning( | ||
| f"Requested max_time override for {self}, but no <max-time .../> tag " | ||
| f"found in {config_path}") | ||
| return | ||
| if len(matches) > 1: | ||
| logging.warning( | ||
| f"Multiple <max-time> tags found in {config_path}; overriding all to {self.max_time}") | ||
| new_text = re.sub(pattern, rf"\g<1>{self.max_time}\g<3>", text) | ||
| try: | ||
| config_path.write_text(new_text) | ||
| logging.info(f"Overwrote max-time in {config_path} to {self.max_time} for {self}") | ||
| except Exception as e: | ||
| logging.warning(f"Failed to write updated {config_path}: {e}") | ||
|
|
There was a problem hiding this comment.
Note that max-time is not the only way to restrict the time. There is also max-time-windows.
https://precice.org/configuration-xml-reference.html#max-time
https://precice.org/configuration-xml-reference.html#max-time-windows
I would leave it up to the user to set the right thing, and not add so much error handling. I think this function is a bit more complicated than it needs to be.
Note that an alternative approach would be to extend the docker compose service before running. We already set some of these in the template, e.g., to always export VTK files:
https://github.com/precice/tutorials/blob/develop/tools/tests/docker-compose.template.yaml
There was a problem hiding this comment.
Actually, for shorter tests, it would be convenient to directly define max-time-windows, instead of max-time, because then we don't need to look at the time-window-size to decide on the value.
As a maintainer, I would be interested to know that, e.g., three coupling time windows complete, since most problems appear there. The time is more relevant for the application, not for the coupling.
| name: str | ||
| cases_of_tutorial: Dict[Tutorial, List[CaseCombination]] | ||
| reference_results: Dict[Tutorial, List[ReferenceResult]] | ||
| max_times: Dict[Tutorial, List[Optional[float]]] = field(default_factory=dict) |
There was a problem hiding this comment.
Similarly to #735: Why is this a (dictionary of a) list per tutorial, and not a value per CaseCombination?
3 participants
Adds per-test max_time override in tests.yaml so individual tests can cap preCICE simulation time without manually editing precice-config.xml.
What changed:
Systemtest: max_time is a proper dataclass field (not set externally after construction)
__apply_precice_max_time_override() applies the override after copying the tutorial, before running — works for both run() and run_for_reference_results() so reference generation is consistent with test runs
Handles multiple tags with a warning instead of silently ignoring them
TestSuite parses max_time from tests.yaml and passes it through everywhere including generate_reference_results.py
Usage:
yamlopenfoam_adapter_pr:
tutorials:
- path: perpendicular-flap
case_combination:
- fluid-openfoam
- solid-calculix
reference_result: ./perpendicular-flap/reference-results/...
max_time: 0.01
Closes #402